Don't allow static items to have types with destructors #11979

flaper87 · 2014-02-01T22:02:31Z

This pull request partially addresses the 2 issues listed before. As part of the work required for this PR, NonCopyable was completely removed.

This PR also replaces the content of type_is_pod with TypeContents::is_pod, although type_is_content is currently not being used anywhere. I kept it for consistency with the other functions that exist in this module.

cc #10834
cc #10577

Proposed static restrictions

Taken from this comment.

I expect some code that, at a high-level, works like this:

For each mutable static item, check that the type:
- cannot own any value whose type has a dtor
- cannot own any values whose type is an owned pointer
For each immutable static item, check that the value:
- does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things, for now)
- does not contain a struct literal or call to an enum variant / struct constructor where
  - the type of the struct/enum is freeze
  - the type of the struct/enum has a dtor

flaper87 · 2014-02-01T22:04:09Z

@nikomatsakis r?

flaper87 · 2014-02-01T22:05:28Z

I just realized the tests got lost somewhere. I'll add them right away.

alexcrichton · 2014-02-02T07:21:23Z

src/librustc/middle/kind.rs

+                let tcontents = ty::type_contents(cx.tcx, self_ty);
+                if tcontents.is_static(cx.tcx) && tcontents.has_dtor() {
+                    cx.tcx.sess.span_err(self_type.span,
+                        "Types with destructors can't be static items");


We're trying to have compiler errors start with lowercase letters.

Didn't know that, Consider it done!

bill-myers · 2014-02-02T22:02:50Z

Why?

Process exit will tear down most resources just fine (otherwise the OS is broken), so there is usually no issue in allowing them and simply never running the destructor.

For things that aren't torn down by process exit, one could add a new Flush trait that gets called using the C++ global destructor mechanism, but only takes an &mut self, is idempotent and does not destroy the object (so that there are no safety issues related to accessing already destroyed global objects in global object destructors).

alexcrichton · 2014-02-03T04:36:19Z

We have been very opposed to "life before main" in terms of static initialization, and we're similarly taking the stance of being opposed to "life after main" in terms of static destruction. For us, once you've made the decision to never run destructors on static variables, we figured it reasonable to just forbid it altogether.

alexcrichton · 2014-02-03T04:39:19Z

Can you also add a test for an owned type? Some types I would not want in a static:

~str
struct Foo { inner: ~str }
struct Foo { inner: ValueWithDestructor }

Just to make sure that it's not just checking that the type itself has a destructor but rather it requires a destructor in general.

flaper87 · 2014-02-03T07:11:21Z

@alexcrichton done. Tests with ~str raise cannot allocate vectors in constant expressions so, I just added one with inner value with destructor.

alexcrichton · 2014-02-03T08:28:41Z

Hm, "cannot allocate vectors in constant expressions" is a different error than this would be expecting, what happens if you try to make a static of type Option<~str>?

flaper87 · 2014-02-03T16:55:17Z

@alexcrichton same error! I think something else is going on there! :/

EDIT: And it's not related to this patch, I'd say. That error is raised also with master.

alexcrichton · 2014-02-03T18:01:24Z

This code compiles today, and I just want to make sure that this patch prevents it from compiling:

static foo: Option<~str> = None;

fn main() {}

nikomatsakis · 2014-02-03T18:48:39Z

src/librustc/middle/kind.rs

+            ItemStatic(self_type, _, _) => {
+                let self_ty: ty::t = ty::node_id_to_type(cx.tcx, item.id);
+                let tcontents = ty::type_contents(cx.tcx, self_ty);
+                if tcontents.is_static(cx.tcx) && tcontents.has_dtor() {


Per @alexcrichton's comment, I imagine the correct rule here is simply to check that all static things are POD. This would prohibit Option<~T>, destructors, etc., as well as managed data, which just seems like trouble waiting to happen.

This might not be good -- it would rule out static atomics, if we want those. I guess the real answer we want is something more like type_needs_drop but that is a codegen concept, not a language exposed thing (as of yet).

Why do atomics need a destructor? Can they just be marked with NoPod now?

See #12016 (destructor => NoPod for atomics)

nikomatsakis · 2014-02-03T18:53:33Z

I guess the right thing is just to define a category: "safe for static" or something, which is equivalent to !type_needs_drop today. That is, no owned data, no dtors, and no managed data.

nikomatsakis · 2014-02-03T21:29:47Z

Can you add a case for putting Option<GC<T>> etc into a static?

flaper87 · 2014-02-03T21:36:15Z

@nikomatsakis done! thanks!

flaper87 · 2014-02-05T22:18:40Z

Something interesting came up.

The summary of the fix for this issue is that static items won't be allowed for types that own destructors, managed pointers or owned pointers. Although this rule seems reasonable, we've hit some cases where abusing of the lack of this rule was actually useful.

The first case, and perhaps the least invasive one, is the DUMMY_SP static item defined in codemap. It has a field that contains a managed pointer, hence it can't be the type of a static item. This case was replaced with a function DUMMY_SP() and all the references to that static item where changed.

The second case is for the static items defined in rustc::middle::ty . Although this case is very similar to the previous one, it's different in terms of the values being stored in the static item. In this case the enum type sty has a ty_tuple(~[T]) variant, however the static items created don't store that variant.

This last case is an example that suggest enforcing static restrictions on the values as opposed of types as a better solution.

After chatting with @nikomatsakis on IRC, it seems that enforcing these rules on the values is not especially hard. There are some cases to consider for static mut items. We could loosen the rules for immutable static items and make them harder for mutable static items.

I've pushed the latest version of the work. As already mentioned, DUMMY_SP was replaced with DUMMY_SP() whereas the static items in rustc::middle::ty haven't been changed.

Thoughts?

brson · 2014-02-11T00:40:04Z

Is this mergeable now, with potentially more conservative rules than we might want later?

flaper87 · 2014-02-18T15:01:15Z

@nikomatsakis r?

alexcrichton · 2014-02-18T16:33:59Z

src/librustc/middle/typeck/check/mod.rs

+            let mutable = match mutability {
+                ast::MutImmutable => {false}
+                ast::MutMutable => {true}
+            };


Why not pass around mutability instead of converting it to a bool? It's self-documenting when you match on it vs having to name it mutable

I had an implementation that passed around mutability, I don't recall why I changed it, TBH. I can change it back

nikomatsakis · 2014-02-21T20:08:08Z

This isn't quite setup how I expect.

I expect some code that, at a high-level, works like this:

For each static mut item, check that the type:
- cannot own any value whose type has a dtor
- cannot own any values whose type is an owned pointer
- anything else?
For each static item, recurse over the expression and check that it:
- does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things, for now)
- does not contain a struct literal or call to an enum variant / struct ctor where:
  - the type of the struct/enum is not freeze
  - the type of the struct/enum has a dtor

Note that we do not examine the type of static imm items at all. The necessary restrictions are all implied by checking the initializer expression instead.

flaper87 · 2014-02-21T20:28:36Z

@nikomatsakis thanks for your comment. I now realized that I could've implemented this in other way. I'll revamp this patch.

nikomatsakis · 2014-02-24T18:34:16Z

@flaper87 ping me when I should re-review

alexcrichton · 2014-02-26T23:59:06Z

src/librustc/middle/check_static.rs

+            } else {return None;}
+        }
+        _ => {}
+    }


This seems... odd. Structs with destructors are allowed, but enums aren't?

Structs destructors are checked in line L46. The reason we can't use type contents for enums is that it will also consider the variant constructors types and whether those types have a destructor, which is not good for things like this

That sounds like for immutable statics you want to not use the type contents, but for mutable statics you do indeed want to check the type contents.

With this in mind, can you add a test for this:

enum A { A1(~str), B } static mut foo: A = B; //~error: `A` has a destructor

alexcrichton · 2014-02-27T00:02:36Z

src/librustc/middle/check_static.rs

+                ast::ExprCall(..) => {
+                    visit::walk_expr(self, e, is_const);
+                    // We also want to check if the enum
+                    // has a destructor


I think tuple struct constructors are also calls, so the comment shouldn't just single out enums

nikomatsakis · 2014-02-27T01:02:49Z

ok, I left some comments -- much nicer, almost there but still a bit more refactoring to go I think :)

- For each *mutable* static item, check that the **type**: - cannot own any value whose type has a dtor - cannot own any values whose type is an owned pointer - For each *immutable* static item, check that the **value**: - does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things) - does not contain a struct literal or call to an enum variant / struct constructor where - the type of the struct/enum has a dtor

This pull request partially addresses the 2 issues listed before. As part of the work required for this PR, `NonCopyable` was completely removed. This PR also replaces the content of `type_is_pod` with `TypeContents::is_pod`, although `type_is_content` is currently not being used anywhere. I kept it for consistency with the other functions that exist in this module. cc #10834 cc #10577 Proposed static restrictions ===================== Taken from [this](#11979 (comment)) comment. I expect some code that, at a high-level, works like this: - For each *mutable* static item, check that the **type**: - cannot own any value whose type has a dtor - cannot own any values whose type is an owned pointer - For each *immutable* static item, check that the **value**: - does not contain any ~ or box expressions (including ~[1, 2, 3] sort of things, for now) - does not contain a struct literal or call to an enum variant / struct constructor where - the type of the struct/enum is freeze - the type of the struct/enum has a dtor

huonw · 2014-02-28T04:57:11Z

Does the infrastructure added here make addressing #5244 easier?

(Awesome work implementing this, BTW.)

add configuration for [`wildcard_imports`] to ignore certain imports fixes: rust-lang#11428 changelog: add configuration `ignored-wildcard-imports` for lint [`wildcard_imports`]

alexcrichton reviewed Feb 2, 2014
View reviewed changes

nikomatsakis reviewed Feb 3, 2014
View reviewed changes

nikomatsakis mentioned this pull request Feb 5, 2014

Attempting to move a non-copyable static segfaults #10577

Closed

huonw mentioned this pull request Feb 14, 2014

ICE when a static variable is declared of type for which the Drop trait is implemented #9243

Closed

alexcrichton reviewed Feb 18, 2014
View reviewed changes

alexcrichton reviewed Feb 26, 2014
View reviewed changes

alexcrichton reviewed Feb 27, 2014
View reviewed changes

flaper87 added 8 commits February 27, 2014 09:24

Remove unused type_is_pod function

4cb3bd5

Implement has_dtor method in TypeContents

a22539b

Keep track of Enum's with destructors

c75dd78

Forbid moves out of static items Closes rust-lang#10577

8784d2f

Closes rust-lang#9243 Test case

49d5d70

Closes rust-lang#7364 Test case

0c7a012

Immutable static items should be Freeze Fixes rust-lang#12432

59a04f5

bors merged commit 59a04f5 into rust-lang:master Feb 28, 2014

alexcrichton mentioned this pull request Mar 3, 2014

Static mutable enum variables break the compiler. #11516

Closed

flaper87 deleted the static branch March 11, 2014 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't allow static items to have types with destructors #11979

Don't allow static items to have types with destructors #11979

flaper87 commented Feb 1, 2014

flaper87 commented Feb 1, 2014

flaper87 commented Feb 1, 2014

alexcrichton Feb 2, 2014

flaper87 Feb 2, 2014

bill-myers commented Feb 2, 2014

alexcrichton commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

flaper87 commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

flaper87 commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

nikomatsakis Feb 3, 2014

nikomatsakis Feb 3, 2014

thestinger Feb 3, 2014

alexcrichton Feb 3, 2014

nikomatsakis commented Feb 3, 2014

nikomatsakis commented Feb 3, 2014

flaper87 commented Feb 3, 2014

flaper87 commented Feb 5, 2014

brson commented Feb 11, 2014

flaper87 commented Feb 18, 2014

alexcrichton Feb 18, 2014

flaper87 Feb 18, 2014

nikomatsakis commented Feb 21, 2014

flaper87 commented Feb 21, 2014

nikomatsakis commented Feb 24, 2014

alexcrichton Feb 26, 2014

flaper87 Feb 27, 2014

alexcrichton Feb 27, 2014

alexcrichton Feb 27, 2014

flaper87 Feb 27, 2014

nikomatsakis commented Feb 27, 2014

huonw commented Feb 28, 2014

Don't allow static items to have types with destructors #11979

Don't allow static items to have types with destructors #11979

Conversation

flaper87 commented Feb 1, 2014

Proposed static restrictions

flaper87 commented Feb 1, 2014

flaper87 commented Feb 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bill-myers commented Feb 2, 2014

alexcrichton commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

flaper87 commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

flaper87 commented Feb 3, 2014

alexcrichton commented Feb 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Feb 3, 2014

nikomatsakis commented Feb 3, 2014

flaper87 commented Feb 3, 2014

flaper87 commented Feb 5, 2014

brson commented Feb 11, 2014

flaper87 commented Feb 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Feb 21, 2014

flaper87 commented Feb 21, 2014

nikomatsakis commented Feb 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Feb 27, 2014

huonw commented Feb 28, 2014